Use handle-based text overwrites in FileHelper by mnriem · Pull Request #5353 · github/spec-kit

mnriem · 2026-11-09T23:05:31Z

Description

Use anchored directories and opened file descriptors for FileHelper.overwrite_text() while retaining its non-atomic text-upsert contract. Preserve supported contained symlink chains, trusted root aliases, existing inode and hard-link identity, permissions, and native text encoding and newline behavior.

Extract the existing Windows handle primitive into a shared private module, add overwrite coverage, and document the operation's filesystem behavior and limitations. This is an independent change targeting main, not a stacked PR.

Testing

Set up this checkout's .venv and editable install using the local development guide. Run commands from the checkout root.

Tested locally with .venv/bin/specify --help (Windows PowerShell: .\.venv\Scripts\specify.exe --help)
Ran relevant existing tests with .venv/bin/python -m pytest tests -q (Windows PowerShell: .\.venv\Scripts\python.exe -m pytest tests -q); use affected test paths for focused checks and LC_ALL=en_US.UTF-8 on macOS
Tested with a sample project (if applicable)

Commands and results:

.venv/bin/specify --help — passed.
LC_ALL=en_US.UTF-8 .venv/bin/python -m pytest tests/test_file_helper.py tests/test_file_helper_overwrite.py tests/test_file_handles.py tests/specify_cli/workflows/test_catalog_cache.py tests/specify_cli/test_file_policy.py tests/integrations/test_integration_state.py -q — 489 passed, 88 skipped.
git diff --check — passed.

Validation ran on macOS. Native Windows tests are included but were not executed on this host. A separate sample-project run is not applicable; operation tests use temporary project roots. The full test suite was not run.

AI Disclosure

I did not use AI assistance for this contribution
I did use AI assistance (fill in the disclosure below)

AI disclosure: GitHub Copilot using GPT-6.1 Sol (model ID: gpt-6.1-sol) in autonomous mode with runtime-default reasoning settings authored the implementation, tests, documentation, commit message, and PR description, and ran local validation on behalf of @mnriem.

Anchor resolved overwrite destinations while retaining in-place updates, supported contained links, and native text behavior. Reuse shared Windows handle primitives and add overwrite regression coverage and contract documentation. Assisted-by: GitHub Copilot (model: GPT-6.1 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot

🟡 Changes recommended

The file-opening changes bypass Windows creation masks, reject otherwise valid writes beneath unreadable parent directories, and break existing Windows permission-failure tests.

3 open issues

What changed in this PR

This PR moves FileHelper.overwrite_text() to handle-based I/O to resist link-based path swaps while retaining non-atomic, in-place text updates.

Changes:

Anchor directory traversal and validate the opened file before truncation.
Share Windows handle utilities with workflow catalog caching.
Add overwrite and handle tests, and document filesystem guarantees and limitations.

Validation here used isolated POSIX checks; native Windows behavior was not executed.

File	Description
`tests/test_file_helper_overwrite.py`	Covers path swaps, text behavior, metadata preservation, and cleanup.
`tests/test_file_handles.py`	Checks Windows handle flags, validation, and ownership.
`src/specify_cli/workflows/_catalog_cache.py`	Reuses the extracted Windows handle helper.
`src/specify_cli/file_helper.py`	Implements anchored, descriptor-based text overwrites.
`src/specify_cli/_file_handles.py`	Provides shared Windows handle and overwrite-descriptor primitives.
`design/file-helper.md`	Documents overwrite behavior, platform requirements, and safety limits.

_{Review effort: Max (auto)}

Honor the Windows creation mask, retain access through searchable unreadable POSIX directories, and inject overwrite failures at the shared descriptor boundary. Cover native creation-mask parity and directory permission success and failure cases. Assisted-by: GitHub Copilot (model: GPT-6.1 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Use the Darwin kernel search bit when Python does not expose O_SEARCH or O_PATH, and exercise the missing-constant branch for existing and new overwrite destinations. Assisted-by: GitHub Copilot (model: GPT-6.1 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Compare native creation with the overwrite API's UTF-8 default explicitly, including on Python versions whose Windows locale encoding differs. Assisted-by: GitHub Copilot (model: GPT-6.1 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot

🟡 Changes recommended

Concurrent Windows overwrites can corrupt the process-wide umask, POSIX traversal can exhaust file descriptors, and the Windows regression test can fail because it compares different encodings.

3 open issues

2 previously missed issues

In code that hasn't changed since last review

Close previous POSIX directory FDs during traversal

src/specify_cli/file_helper.py:267

Keeping every POSIX ancestor in the ExitStack makes descriptor usage grow with path depth, although only the current directory FD is needed for anchored traversal. With 80 valid parent directories and RLIMIT_NOFILE=64, native text overwrite succeeds but this implementation raises EMFILE before opening the leaf. Please close each previous parent after opening its child, as _cache_directory already does; retain the full hierarchy only on Windows, where the sharing locks require it.

Remove the blanket overwrite_text sandbox exception

design/file-helper.md:227

The blanket overwrite_text exception implies a race-free sandbox and excludes its writes from the partial-results warning. That conflicts with its write-failure contract at lines 180–182 and namespace limits at lines 242–246. Remove this exception and keep the narrower anchored-traversal guarantee in the operation's description above.

2 resolved since last review

_{Review effort: Max (auto)}

Let exclusive CRT creation apply Windows creation masks while retaining anchored existing-file opens. Close previous POSIX directory descriptors during traversal and clarify overwrite limitations. Cover interleaved calls, deep paths under low descriptor limits, and Windows creation collisions. Assisted-by: GitHub Copilot (model: GPT-6.1 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot

🔵 Needs a closer look

Overlapping overwrites can corrupt the Windows runtime permission mask, while directory pins unnecessarily require listing access and block otherwise permitted writes.

2 open issues

2 previously missed issues

In code that hasn't changed since last review

Overlapping overwrites corrupt the process creation mask

src/specify_cli/_file_handles.py:46

Two overlapping overwrites can restore 0o200 over an original mask of 0, making a newly created file read-only and changing permissions for later file creations. Unrelated native text-file creation can also observe the temporary mask between these calls. Use a creation strategy that applies the Windows runtime's mask without mutating it, and add an overlapping-call regression; a helper-only lock would not protect unrelated file creators.

Directory pins incorrectly require list access

src/specify_cli/file_helper.py:239

The reused _windows_handle(..., directory=True) requests FILE_LIST_DIRECTORY for every directory it pins. On Windows, a directory can allow traversal and writes to a known child while denying directory listing, so this now raises PermissionError for files that native Path.write_text() and the previous implementation can overwrite. Please use directory pins that require traversal rather than listing access while retaining rename protection, and add a native Windows ACL parity regression.

1 resolved since last review

Specify UTF-8 in the native reference write

_{Review effort: Max (auto)}

Capture masked Windows creation attributes with an empty private probe in the pinned parent, then exclusively create the destination through a no-follow native handle. Clean up the probe, preserve existing in-place updates, and cover leaf collisions during attribute capture. Assisted-by: GitHub Copilot (model: GPT-6.1 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot

🟡 Changes recommended

A critical Windows symlink race can redirect exclusive file creation outside the helper root, violating the no-redirection guarantee.

2 open issues

1 resolved since last review

Avoid process-wide umask changes during Windows overwrites

_{Review effort: Max (auto)}

Copilot

🟡 Changes recommended

The Windows permission probe can fail when creation and writing are allowed but deletion is denied, leaving a probe file behind and aborting otherwise permitted destination creation.

2 open issues

1 resolved since last review

Prevent Windows exclusive-create from following dangling symlinks

_{Review effort: Max (auto)}

Discover CRT-masked creation attributes in independent runtime-owned temporary storage instead of a disposable project sibling. Preserve native no-follow destination creation and cover create/write-without-delete directory ACLs. Assisted-by: GitHub Copilot (model: GPT-6.1 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Resolve shared handle and filesystem contract conflicts while retaining the upstream exclusive-creation implementation and both test suites. Assisted-by: GitHub Copilot (model: GPT-6.1 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Apply create/write-without-delete permissions to the child destination directory, keeping root canonicalization separate from the overwrite permission regression. Assisted-by: GitHub Copilot (model: GPT-6.1 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot

🔵 Needs a closer look

Human assessment is needed because Windows directory pinning can reject otherwise permitted text writes when ACLs deny directory listing.

1 open issue

Update Windows overwrite failure injection in tests

1 resolved since last review

Avoid requiring delete permission to determine creation attributes

_{Review effort: Max (auto)}

Deny the advanced DE and DC rights explicitly and confirm ordinary file reading remains available before exercising overwrite behavior. Assisted-by: GitHub Copilot (model: GPT-6.1 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot

🟡 Changes recommended

The overwrite documentation omits Windows pinning’s directory-listing and attribute-read requirements on the canonical root and resolved parents, leaving unexplained why restrictive access rules can prevent overwriting an otherwise writable file.

2 open issues

_{Review effort: Max (auto)}

Copilot

🔵 Needs a closer look

The overwrite contract omits the new Windows directory-list access requirement on the root and parent directories, leaving a platform-specific failure mode undocumented.

2 open issues

_{Review effort: Max (auto)}

State that the canonical root and every resolved parent require directory-list and read-attributes access, including the resulting failure when an ACL allows file writes but denies listing. Assisted-by: GitHub Copilot (model: GPT-6.1 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot

🔵 Needs a closer look

Human assessment is needed because the FIFO test can pass without O_NONBLOCK, leaving the no-reader case vulnerable to an undetected indefinite hang.

1 open issue

Update Windows overwrite failure injection in tests

1 resolved since last review

Document Windows directory-listing access requirement

_{Review effort: Max (auto)}

Exercise leaf replacement with a no-reader FIFO in timeout-bounded subprocesses for both link policies. Require the nonblocking ENXIO failure and preserve the original file contents while retaining reader-present file-type coverage. Assisted-by: GitHub Copilot (model: GPT-6.1 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot

🔵 Needs a closer look

Human review is needed to assess test coverage gaps around unsupported platform primitives, concurrent file creation, and creation-mask probe avoidance during Windows overwrites.

1 open issue

Update Windows overwrite failure injection in tests

_{Review effort: Max (auto)}

Exercise missing and zero-valued primitives, regular files introduced before POSIX open, and Windows existing-file probe avoidance. Check explicit failures and preserve inode, hard-link, and permission behavior. Assisted-by: GitHub Copilot (model: GPT-6.1 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot

🔵 Needs a closer look

Human assessment is needed to judge whether the concurrency test’s simulated umask state and timed waits introduce unnecessary complexity.

1 open issue

Update Windows overwrite failure injection in tests

_{Review effort: Max (auto)}

Reuse the direct process-mask assertion while retaining concurrent calls and descriptor cleanup checks. Remove simulated mask state, synchronization events, and timed interleaving waits. Assisted-by: GitHub Copilot (model: GPT-6.1 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

mnriem · 2026-11-10T17:44:08Z

Updated in 032dad0d: the concurrency test now runs two overwrite calls and relies on the existing assertion that os.umask must not be called, with no simulated mask state or timed waits. It still checks descriptor cleanup. .venv/bin/python -m pytest tests/test_file_handles.py tests/test_file_helper.py::test_overwrite_text_io_errors_propagate -q passed (38 tests). GitHub Copilot (GPT-6.1 Sol, gpt-6.1-sol, autonomous; reasoning setting not exposed) generated this test change, ran validation, and wrote this comment on behalf of @mnriem.

Copilot

🔵 Needs a closer look

The new Win32/CRT filesystem paths lack native Windows validation, and macOS-only runs with portable mocks cannot establish sharing, ACL, creation-mask, or handle-ownership behavior.

No open issues 🎉

1 resolved since last review

Update Windows overwrite failure injection in tests

_{Review effort: Max (auto)}

Copilot AI auto review requested due to automatic review settings November 9, 2026 23:05

Copilot started reviewing on behalf of mnriem November 9, 2026 23:06

View session

Copilot AI reviewed Nov 9, 2026

View reviewed changes

Comment thread src/specify_cli/_file_handles.py Resolved

Comment thread src/specify_cli/file_helper.py Resolved

Comment thread src/specify_cli/file_helper.py Resolved

Copilot AI auto review requested due to automatic review settings November 9, 2026 23:31

Copilot started reviewing on behalf of mnriem November 9, 2026 23:32

View session

mnriem and others added 2 commits November 9, 2026 17:42

Copilot AI reviewed Nov 9, 2026

View reviewed changes

Comment thread src/specify_cli/_file_handles.py Resolved

Comment thread tests/test_file_helper_overwrite.py Resolved

Copilot AI auto review requested due to automatic review settings November 9, 2026 23:53

Copilot started reviewing on behalf of mnriem November 9, 2026 23:54

View session